Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MISC] Allow options for subcommands #244

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Feb 27, 2024

Follow-up of #238

Main Problem

The subcommand is determined by just checking whether any argument matches a subcommand.
To allow options for the top-level-parser, we need to know whether a potential subcommand is actually an option value.
E.g.,

// raptor build is a subcommand
raptor -o build

Currently, it would use build as subcommand and then fail on parse() of the top-level-parser.
At this point of parsing, we don't know whether -o is an option or flag, so just checking for - is not enough.

My first intuition is that the format_parse needs to know about (and) handle the subcommands. Which would also mean that it needs a pointer to the parent.
However, special formats also must know to which command they apply.

Since format_parse has functions to check if an option or flag is set, those might be reused.

Test cases

Test case 1

// raptor has option `-o`
// raptor-build has option `-o`
raptor build -o foo
// `-o` belongs to build

Test case 2

// raptor has option `-o`
// raptor-build has option `-o`
raptor -o build build -o build
// first `-o` belongs to raptor, second to raptor-build

Test case 3

// raptor has option `-o`
// raptor-build has option `-o`
raptor -o build buidl -o build
// Wrong subcommand!

Test case 4

// raptor has option `-o`
// raptor-build has option `-o`
raptor -o=build build -o=build
// first `-o` belongs to raptor, second to raptor-build

Test case 5

// raptor has flags `-{b,u,i,l,d}`
// raptor-build has option `-o`
raptor -build build -o build
// `-build` are flags for raptor, `-o build` an option for raptor-build

Test case 6

// raptor has option `-o`
// raptor-build has option `-o`
raptor -o build // raptor short-help
raptor -o build --help // raptor full-help
raptor -o build build // raptor-build short-help
raptor -o build build --help // raptor-build full-help
raptor build // raptor-build short-help
raptor build --help // raptor-build full-help
raptor -o build build -o build --help // raptor-build full-help

Test case 7

// --option is an option
// What should happen?
raptor --option --help

Copy link

vercel bot commented Feb 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
sharg-parser ✅ Ready (Inspect) Visit Preview Mar 1, 2024 9:43pm

@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Feb 27, 2024
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (01f848e) to head (d44f6a1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
- Coverage   95.13%   95.10%   -0.04%     
==========================================
  Files          18       18              
  Lines        1728     1735       +7     
==========================================
+ Hits         1644     1650       +6     
- Misses         84       85       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from 820e72d to e6ef526 Compare February 29, 2024 10:16
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Feb 29, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from e6ef526 to b0acb3e Compare March 1, 2024 12:15
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from b0acb3e to 35c537c Compare March 1, 2024 14:35
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from 35c537c to 700591e Compare March 1, 2024 15:58
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from 700591e to 9cb2421 Compare March 1, 2024 16:06
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from 9cb2421 to 3b9fee0 Compare March 1, 2024 16:10
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler marked this pull request as ready for review March 1, 2024 16:21
@eseiler eseiler requested review from smehringer and SGSSGene March 1, 2024 16:21
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two things:

  1. std::vector<std::string> options is used as a set, so maybe std::unordered_set<std::string> is the appropriate data structure.
  2. I personally prefer 'unknown subcommand' over 'misspelled subcommand'

include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
include/sharg/parser.hpp Outdated Show resolved Hide resolved
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from d89995c to 8f755cc Compare March 1, 2024 21:28
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from 8f755cc to a236f78 Compare March 1, 2024 21:29
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler
Copy link
Member Author

eseiler commented Mar 1, 2024

I see two things:

  1. std::vector<std::string> options is used as a set, so maybe std::unordered_set<std::string> is the appropriate data structure.
  2. I personally prefer 'unknown subcommand' over 'misspelled subcommand'

Agreed.

I ended up refactoring verify_identifiers while trying to not store options in both used_ids and options. I also ended up that this would be a follow-up if at all :D

@eseiler eseiler requested a review from SGSSGene March 1, 2024 21:33
@eseiler eseiler force-pushed the misc/allow_options_subcommand branch from a236f78 to d44f6a1 Compare March 1, 2024 21:42
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Mar 1, 2024
@eseiler eseiler removed the request for review from smehringer March 13, 2024 12:53
@eseiler eseiler merged commit 39f65a4 into seqan:main Mar 13, 2024
33 checks passed
@eseiler eseiler deleted the misc/allow_options_subcommand branch March 13, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants